Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5025d2e | 1248.52 ms | 1251.72 ms | 3.20 ms |
| b869536 | 1250.37 ms | 1274.84 ms | 24.47 ms |
| 5025d2e | 1245.14 ms | 1268.58 ms | 23.44 ms |
| 9fc2dd0 | 1246.14 ms | 1275.00 ms | 28.86 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 5025d2e | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| b869536 | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| 5025d2e | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| 9fc2dd0 | 20.50 KiB | 331.79 KiB | 311.28 KiB |
Previous results on branch: fix/bytes-type
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a5014e6 | 1247.04 ms | 1261.68 ms | 14.64 ms |
| e22634b | 1205.16 ms | 1233.28 ms | 28.12 ms |
| 1e02273 | 1228.39 ms | 1257.07 ms | 28.68 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| a5014e6 | 20.50 KiB | 331.79 KiB | 311.29 KiB |
| e22634b | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| 1e02273 | 20.51 KiB | 331.79 KiB | 311.28 KiB |
There was a problem hiding this comment.
There are still usages of uint64_t in SentryCrashMonitor_System.m that would intermix with this typedef, this only got about half of them. Did you intend to not change those?
Nice change overall. Relatedly, one of the only times I like using a sort of Hungarian notation is to put units like _bytes/_mb/_ms/_minutes/_degrees/_radians on variable/function names. Also relatedly, one of my favorite reasons to use Swift is to be able to use https://github.com/pointfreeco/swift-tagged for even better type safety around this kind of thing.
|
I must've missed those, I'll take a look tomorrow. And yea I'm a huge fan of everything the Point-Free guys do :) |
|
TBH Im not a huge fan of this change. The result information should be clear in the documentation, and the function name should be clear. Having Take |
|
We can do both. If we call it |
|
IMO a simple Above the function in the header should be enough to solve this. That being said, I will not oppose if you want to merge this. Just sharing my thoughts. |
|
We could always add the maybe overkill, just looking at alternatives. I'm mostly fine with the proposed typedef. One benefit of having the enum/struct is that doc comments can be attached that can be brought up with a doc popover in Xcode with an ⌥ + click on the names. |
* master: release: 7.26.0 meta: Fix Changelog concurrent transactions (#2229) build(deps): bump fastlane from 2.210.0 to 2.210.1 (#2224) Revert "feat: profile concurrent transactions (#2105)" (#2225) fix: Align core data span operations (#2222) test: Remove empty assert msg for AppStateTests (#2221) meta: Fix Changelog (#2219) ref: Add typealias for bytes (#2209) feat: profile concurrent transactions (#2105) ci: Readd cache for UI tests (#2215) # Conflicts: # Sentry.xcodeproj/project.pbxproj
This should make it clear what kind of size these variables are. Are they KB, MB, GB? Nope, they are all bytes.
#skip-changelog